-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
utils
and utils.create_from_yaml
improvements
#783
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oz123 If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
performe an action from a valid parsed yaml object (as dict). | ||
|
||
:param k8s_client: an ApiClient objcet initialized with the client args | ||
:yml_object dict: a parsed yaml object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think one needs to explicitly reference yaml here as what the function wants is a kubernetes manifest in python dictionary form
though it probably also does not harm as yaml is the most common representation form of that data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is why I added create_from_map
/cc @micw523 |
I think I'm still holding my original position in #722: we don't need many function names. We should be able to do everything in Also, we're pushing to merge 673, so it's likely that you'll need a rebase soon... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
92ed9d4
to
c257a38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm still holding my original position in #722: we don't need many function names. We should be able to do everything in
create_from_yaml
(potentially with an extra input parameter), since the wordyaml
isn't specific to a file anyway. If absolutely needed we can make an alias oncreate_from_yaml
.Also, we're pushing to merge 673, so it's likely that you'll need a rebase soon...
I agree with you about creating an alias. IMHO it's better to be explicit . Since "yaml" isn't necessary a file it's better to specify when expect a file\stream and specify when we don't.
Concerning backwards compatibility, I now suggest the following, check if yaml_path is a file on the filesystem, if it is, proceed as in the past. If it is a string, try and parse it to yaml object.
test-requirements.txt
Outdated
isort | ||
adal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked again:
$ grep -nr adal kubernetes | grep -v \.pyc
kubernetes/base/config/kube_config.py:24:import adal
kubernetes/base/config/kube_config.py:223: context = adal.AuthenticationContext(
jwt is a dependency of adal, and I'm not sure adal is specifically required here.
I removed jwt. How do you want me to proceed with adal?
36de28b
to
ce11548
Compare
Which branch did you fork it from? If you forked it from the current master I’ll need to do some investigation. Adal as a requirement got moved to optional in a previous PR so that might be the problem. |
I forked from the master on a fresh clone I did yesterday. |
I just had a fresh fork from the master branch. adal is not required for testing. Is it breaking anything that you submitted? |
I checked your past Travis builds. It doesn't seem like adal is affecting you. Please remove the added packages. |
/assign |
This keeps the function backwords compatible. Changed the parameter yaml to yml in order not to mask the imported module.
done. I completely removed that commit. |
@micw523 the new master branch includes |
I’m not sure. The Do you still want to work on this, or you’re going to leave it in my hands? |
@micw523 I would prefer starting on a clean branch and a clean PR. This has become too opaque for me. Do you mind if I submit a new PR? |
That’s cool, go for it :) |
As discussed in #722.
Seems this had quite some positive feedback.